src: remove misplaced windows code under posix guard in node.cc by thisalihassan · Pull Request #52545 · nodejs/node

thisalihassan · 2024-04-15T18:58:55Z

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check.

Fixes: #52404
Refs: #35033

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: nodejs#52404 Refs: nodejs#35033

nodejs-github-bot · 2024-04-15T18:59:00Z

Review requested:

@nodejs/startup

src/node.cc

daeyeon · 2024-04-19T02:39:50Z

src/node.cc

-    // Tell V8 to disable emitting WebAssembly
-    // memory bounds checks. This means that we have
-    // to catch the SIGSEGV/SIGBUS in TrapWebAssemblyOrContinue
-    // and pass the signal context to V8.


I think this comment is valid still. Why don't we leave it?

sure I will revert this change

src/node.cc

nodejs-github-bot · 2024-04-19T22:10:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58532/

targos · 2024-04-21T06:43:13Z

Looks like it doesn't work on x86:

00:18:01 C:\workspace\node-compile-windows\node\src\node.cc(685,9): error C2664: 'PVOID AddVectoredExceptionHandler(ULONG,PVECTORED_EXCEPTION_HANDLER)': cannot convert argument 2 from 'LONG (__cdecl *)(EXCEPTION_POINTERS *)' to 'PVECTORED_EXCEPTION_HANDLER' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
00:18:01 C:\workspace\node-compile-windows\node\src\node.cc(685,44): message : None of the functions with this name in scope match the target type [C:\workspace\node-compile-windows\node\libnode.vcxproj]
00:18:01 C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\errhandlingapi.h(134,1): message : see declaration of 'AddVectoredExceptionHandler' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
00:18:01 C:\workspace\node-compile-windows\node\src\node.cc(685,9): message : while trying to match the argument list '(const ULONG, overloaded-function)' [C:\workspace\node-compile-windows\node\libnode.vcxproj]

thisalihassan · 2024-04-21T18:52:00Z

Looks like it doesn't work on x86:

00:18:01 C:\workspace\node-compile-windows\node\src\node.cc(685,9): error C2664: 'PVOID AddVectoredExceptionHandler(ULONG,PVECTORED_EXCEPTION_HANDLER)': cannot convert argument 2 from 'LONG (__cdecl *)(EXCEPTION_POINTERS *)' to 'PVECTORED_EXCEPTION_HANDLER' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
00:18:01 C:\workspace\node-compile-windows\node\src\node.cc(685,44): message : None of the functions with this name in scope match the target type [C:\workspace\node-compile-windows\node\libnode.vcxproj]
00:18:01 C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\errhandlingapi.h(134,1): message : see declaration of 'AddVectoredExceptionHandler' [C:\workspace\node-compile-windows\node\libnode.vcxproj]
00:18:01 C:\workspace\node-compile-windows\node\src\node.cc(685,9): message : while trying to match the argument list '(const ULONG, overloaded-function)' [C:\workspace\node-compile-windows\node\libnode.vcxproj]

~~I have AMD64 windows and build is successful with ./vcbuild. Is it failing on 32 Bit?~~

Fixed

nodejs-github-bot · 2024-04-22T22:53:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58619/

thisalihassan · 2024-04-23T05:53:29Z

win-vs2022-arm64
C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.39.33519\include\xtr1common(56,21): fatal error C1060: compiler is out of heap space [C:\workspace\node-compile-windows\node\tools\v8_gypfiles\v8_initializers.vcxproj]

node-test-linux-debian12-x64
Exception when executing the batch command : no workspace from node hudson.slaves.DumbSlave[test-softlayer-debian12-x64-1] which is computer hudson.slaves.SlaveComputer@5d7b87a2 and has channel null

I am not sure why Mac tests are failing.

nodejs-github-bot · 2024-04-23T06:44:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58626/

nodejs-github-bot · 2024-04-29T20:06:34Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58807/

nodejs-github-bot · 2024-04-30T09:53:37Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58816/

nodejs-github-bot · 2024-04-30T15:28:01Z

CI: https://ci.nodejs.org/job/node-test-pull-request/58819/

joyeecheung · 2024-04-30T17:06:59Z

https://ci.nodejs.org/job/node-test-pull-request/58819/ was orange though it seems the bot is unable to update the status in GitHub properly again...

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: #52404 Refs: #35033 PR-URL: #52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

joyeecheung · 2024-04-30T17:08:45Z

Landed in 5976985

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: #52404 Refs: #35033 PR-URL: #52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: nodejs#52404 Refs: nodejs#35033 PR-URL: nodejs#52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: #52404 Refs: #35033 PR-URL: #52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: nodejs#52404 Refs: nodejs#35033 PR-URL: nodejs#52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: #52404 Refs: #35033 PR-URL: #52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

The V8 WebAssembly trap handler setup for Windows was incorrectly nested within a POSIX conditional compilation block in src/node.cc. This caused the related functions to be effectively non-operational on Windows. The changes involve removing the Windows-specific code from the POSIX section and correctly placing it under the WIN32 check. This fix will ensure that the intended exception handling is active on Windows builds. Fixes: nodejs#52404 Refs: nodejs#35033 PR-URL: nodejs#52545 Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Apr 15, 2024

src: format src/node

ce83aae

thisalihassan marked this pull request as draft April 15, 2024 19:08

thisalihassan marked this pull request as ready for review April 16, 2024 06:33

daeyeon reviewed Apr 19, 2024

View reviewed changes

src: resolve feedback

c3166e8

daeyeon reviewed Apr 19, 2024

View reviewed changes

src/node.cc Show resolved Hide resolved

src: resolve feedback

e16d6f0

daeyeon approved these changes Apr 19, 2024

View reviewed changes

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 19, 2024

src: fix build error on win

2c68fcd

daeyeon added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 22, 2024

joyeecheung approved these changes Apr 29, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 29, 2024

joyeecheung closed this Apr 30, 2024

aduh95 mentioned this pull request Apr 30, 2024

2024-05-02, Version 22.1.0 (Current) #52768

Merged

marco-ippolito mentioned this pull request May 23, 2024

v20.14.0 proposal #53120

Merged

marco-ippolito mentioned this pull request Jun 17, 2024

v20.15.0 proposal #53486

Merged

Uh oh!

Conversation

thisalihassan commented Apr 15, 2024

Uh oh!

nodejs-github-bot commented Apr 15, 2024

Uh oh!

Uh oh!

daeyeon Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

thisalihassan Apr 19, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 19, 2024

Uh oh!

targos commented Apr 21, 2024

Uh oh!

thisalihassan commented Apr 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 22, 2024

Uh oh!

thisalihassan commented Apr 23, 2024

Uh oh!

nodejs-github-bot commented Apr 23, 2024

Uh oh!

nodejs-github-bot commented Apr 29, 2024

Uh oh!

nodejs-github-bot commented Apr 30, 2024

Uh oh!

nodejs-github-bot commented Apr 30, 2024

Uh oh!

joyeecheung commented Apr 30, 2024

Uh oh!

joyeecheung commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thisalihassan commented Apr 21, 2024 •

edited

Loading